fix: Match attribute references against String and Symbol hash keys - #422
fix: Match attribute references against String and Symbol hash keys#422keelerm84 wants to merge 4 commits into
Conversation
Reference stores every attribute path component as a Symbol, but context attribute data keeps whatever key type the application used. JSON.parse makes String keys by default, and create_single_context clones each attribute value shallowly, so nested String keys survive into the SDK. Every site that resolved a reference component against that data compared the two with ==, and :email does not equal "email". Three defects followed from that one mismatch: - A nested private attribute was not redacted, and its path was absent from _meta.redactedAttributes, so private data reached the event recorder. - A nested attribute reference resolved to nil during evaluation, so targeting rules and bucketing on a path such as /profile/email never matched and the flag returned the wrong variation. - A String top-level custom attribute key serialized as null in events. Two helpers in Impl::Context now handle both key types. Value lookups use fetch_attribute, which prefers the Symbol key when a hash holds both forms of a name. Redaction uses same_attribute_name?, which matches both forms, so an ambiguous hash loses both values. Redaction fails closed. same_attribute_name? compares with == before to_s, so the Symbol path allocates nothing. redact_json_value runs once per event.
The contract test service parses harness input with symbolize_names, so nested application data always reaches LDContext.create with symbol keys. No contract test can produce the String keyed shape, and the harness has no way to express a Ruby type distinction. These specs are the only coverage for it. Each of the nine new examples fails against the code before the fix. Also report a redacted reference once when a hash holds both the string and the symbol form of the same name. Both values are redacted, but the reference names one attribute.
fetch_attribute tried the name as given and then its string form. That covers a symbol name against a string keyed hash, which is what the two call sites need, because both pass a Reference component and those are always symbols. A string name got no such treatment. The string form of a string is the same string, so the second lookup repeated the first and a symbol keyed hash read as absent. That is the common shape of context attribute data, so a future caller holding a name as a string would have hit it. Try the exact key, then the other form. same_attribute_name? was already symmetric, since it normalizes both sides with to_s, and the pair now reads consistently.
|
One behaviour this change makes worse, flagged for the reviewer rather than buried. A context can mix a Symbol What changed is what gets emitted. Before, the String twin was unreadable and went out as The values leaked in both cases, since that The same shadowing affects case k
when :kind, :key, :name, :anonymous, :_meta,
'kind', 'key', 'name', 'anonymous', '_meta'
nextPrototyped and green, but deliberately not in this PR, because it widens a security fix. |
The build-linux (jruby-9.4) job started failing with no code change, purely from RuboCop floating 1.89.0 to 1.90.0, which released 2026-08-24. The step emitted 11944 internal cop errors across nearly every file in the repository, including files the branch never touched, and exited 2. The CRuby 3.2, CRuby 3.4, and Windows jobs pass on the same 1.90.0. On JRuby the parallel gem cannot fork, so it runs work in threads rather than worker processes. RuboCop 1.90 began preserving cop instances across files, which is safe per worker process but not across threads, so the reused instances see concurrent processed_source values and report locations from the wrong file. Parallelism buys nothing here. Serial and parallel both take 4.5 seconds over 185 files with the cache disabled. This mirrors the same change in the openfeature-ruby-server SDK.
|
Closing in favour of #423, which takes the opposite and better approach. This PR made string-keyed attributes work: it taught the lookup and redaction sites to match #423 also picks up two things this PR did not: the One thing worth carrying over. This branch found that the mismatch breaks flag evaluation, Replaced by a documentation-only PR that states that requirement in the code, since nothing |
1 participant
Reference stores every attribute path component as a Symbol, but context
attribute data keeps whatever key type the application used. JSON.parse
makes String keys by default, and create_single_context clones each
attribute value shallowly, so nested String keys survive into the SDK.
Every site that resolved a reference component against that data compared
the two with ==, and :email does not equal "email". Three defects followed
from that one mismatch:
from _meta.redactedAttributes, so private data reached the event
recorder.
targeting rules and bucketing on a path such as /profile/email never
matched and the flag returned the wrong variation.
Two helpers in Impl::Context now handle both key types. Value lookups use
fetch_attribute, which prefers the Symbol key when a hash holds both
forms of a name. Redaction uses same_attribute_name?, which matches both
forms, so an ambiguous hash loses both values. Redaction fails closed.
same_attribute_name? compares with == before to_s, so the Symbol path
allocates nothing. redact_json_value runs once per event.